fix(kvstore): override query+fragment from path if they exist when joining with base url #727
Conversation
…ining with base url
|
I apologize for introducing this breakage --- I did some testing of graphene when I landed the initial kvstore refactor but I didn't remember to test it when doing some additional fixes. Basically, the issue is related to the fact that graphene uses arbitrary HTTP APIs, and therefore doesn't actually fit into the kvstore model, but because it was convenient to be able to just register middleauth using the kvstore mechanism, I changed graphene to use the kvstore APIs. The specific issue here is that with the kvstore APIs we have the concept of a path, which is allowed to contain basically any character, including special characters like "|" and "?" and "#", and has no special meaning for "%", while a URL pipeline gives special meaning to certain characters, and to avoid that special meaning percent encoding must be used. For example, you can have a filename literally named "?" served by a webserver, which you can retrieve using the url "%3F". The proposed fix here is problematic because the I created a different fix here. Some basic testing suggests that it fixes the problem, but if you are able to confirm the fix that would be great. Separately, it would be extremely helpful if we can add some mock/fake tests for graphene, similar to the tests in tests/kvstore/ngauth.browser_test.ts, to avoid these breakages in the future. |
Here is a proposed fix for the following issue:
Currently HttpKvStore assumes the query and fragment are in the baseUrl when constructing the full url with
joinBaseUrlAndPath. If it exists in the path, encodePathForUrl causes an incorrect url because the query character is encoded.neuroglancer/src/kvstore/url.ts
Line 205 in 35a4af7
Graphene currently uses the fragment when constructing the path for two endpoints:
neuroglancer/src/datasource/graphene/backend.ts
Line 152 in 35a4af7
neuroglancer/src/datasource/graphene/backend.ts
Line 278 in 35a4af7
For simplicity, I'm just assuming if query/fragment are in the path, we can ignore if the base url contains either.